-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Snippet bar wrapping #3635
base: master
Are you sure you want to change the base?
Snippet bar wrapping #3635
Conversation
The deployment isn't showing any change for me. |
huh i guess the deployment is for some reason ignoring the container property and query? |
Ok can confirm works perfectly fine in local, and there are no changes to push.... |
Fixed it, for some reason less doesn't like container queries nested in deployment. I have reason to believe this would work if our version of less wasn't 1 year behind. |
Okay, confirmed what I suspected might happen (I've made pretty much this exact PR before): If the editor buttons wrap below the dropdown menus, you can't use the dropdown menus. Via JS, the dropdown menus are placed below the snippet bar total height. If you hover over the menu button, then move your mouse down to select something from the menu, your mouse will move over the Editor buttons and thus off your hover-activated menu and close it. I think an appropriate fix, without going overboard on UI changes or pulling in UI libraries, is to rearrange the entire snippet bar-- it would fix this probably and I believe make more logical sense. Basically, flip everything around to reverse order. Move the Editor buttons (Brew, Style, Properties) furthest to left. These are the main categories of the editor and completely change the content. Move the Undo/Redo buttons and codemirror theme selector to the space after the editors. These are applicable to multiple editors (Brew and Style), and do not change. Move the snippet menus to the end of the snippet bar. These are dependent on the active editor, and are further dependent on the active Theme-- some themes may have more or fewer menus. Placing them at the end allows that number to grow without causing as much issue (though the dropdown menu/hover issue will persist if we continue to break the snippet menus across multiple lines). Then allow wrapping at the same spot-- just before the snippet menus. |
I realize now that you already linked to the Issue where I show the re-ordering idea: #2076 (comment) So yeah....I don't know if my old branch even still exists on my computer or if it can be brought up to Master easily. But that's my idea in a giffy nutshell. |
Damn, yeah, i should have given this PR more than 5 minutes... hmm i personally don't like the idea of swapping them, visually, but i guess it is the best we have... Could use the team's opinion on this, @calculuschild @G-Ambatte @ericscheid @dbolack-ab |
Should we fix that first? |
I dunno. I kinda like the controls up the "centerline" of the window, particularly if the scroll-lock/jump buttons are added. This is a somewhat radical suggestion. What about moving the Snippets to a single parent on the left and changing it to a vertical menu? |
Is CSS smart enough that we could leave the order the same, but have the snippets take the bottom row when wrapping? |
Possibly with CSS Grid and grid areas, and @media breakpoints. I think that’s a weird way to fix it, compared to just reordering. @dbolack-ab I’m not sure I have a complete understanding of your first proposal: where would are you suggesting the scroll related buttons go? EDIT I don't think it's a bad idea, except with our current components it adds another point of failure when trying to get through menus. Because our menus are on 'hover' only, it's easy to lose your progress through a menu each time there is another level of depth. I think this approach of a deeper menu tree would be improved with an actual UI library that can provide dropdown menus that stay open on click (and close "on click outside") and can be navigated entirely with keyboard arrows/tabs. One more point, that is more general, is that another level of nesting will decrease discoverability of all snippets-- they'll be further hidden, and it'll be "one click extra" more difficult to just try a bunch. |
Yes, that covers it. And yeah, it is a non-trivial change, but it keeps the top bar clean and in place. The Snippets menu would be better served by being click to open on subs, however many menus have actions on click as well as having subs which means a possibly non-trivial reworking. With the potential of incoming user snippets, it seems like our best bad choice. Also, rotating the menu to being vertical rather than horizontal might help with the mouseover problem. |
Okay, having read it all, can we merge this PR as is, or with items reversed, and leave that to another PR? |
Can't merge as-is because the snippets are still unusable when wrapping happens. "If the editor buttons wrap below the dropdown menus, you can't use the dropdown menus." Need to solve that first. |
UncollapsedCollapsed |
…nto snippet-bar-wrapping
…apper for snippets, changed CSS styles for editors and snippets"
Gotta be honest, i like the unwrapped, but the second line being wider i don't particularly enjoy Also, i am considering addnig names to the tabs, and separating them from the rest of the UI with some sort of colors or moving them to a separate place. |
That's why i stretched the editor buttons in my original take on this, again in this comment: 2076. It is significantly less jarring. I also think that the editor buttons should be labelled, because I hate having to tell new users "Go to the Style Editor (the paint brush icon by the snippets)...". For a critical piece of the editor, it shouldn't only be an icon. To that effect, I had been working on proper looking tabs, separate from the snippets. I have (or had) a branch that widely adopted Radix UI library but I'm not sure I ever published it (there is a closed one, but that was an older attempt) which had editor tabs...I can't find it now. But basically it looked like what is shown in the ToC discussion, here. I still think it is an effective way to communicate that the buttons are not part of the snippets, that they are important, and help users find them. But without straying too far from where this PR is currently at, you could add text labels to the current icons-- and then drop them when their containers get too small, now that CSS Container Queries are a thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few observations with the current behavior:
- Snippets do not wrap on the style tab. They jump over to the left but do not move into their own row. All tabs should probably have the same behavior
- Editor theme dropdown is cut off on the left
- I would personally prefer snippets stay on the left when unfolded if it can be done.
- Our snippet subgroups unfold toward the right, which feels more natural than unfolding to the left or unfolding out on top of the BrewRenderer IMO
- Snippets on the right means the dropdowns go off the edge
- When folded, the snippets become left-aligned anyway. Reworking everything to switch between supporting left and right-alignment depending on the fold state seems more confusion than it's worth.
- I agree with Gazook's comments about making the top buttons resize to fill the top row
Agree/disagree on these?
Before
After
No animations, no nothin, just wrapping when there is no space.